typeck: silence lint for "collision safe" items #99898

davidtwco · 2022-07-29T09:09:19Z

Requested in #97483 (comment) and on Zulip.

When an unstable method exists on a type and the user is invoking a method that would no longer have priority when the unstable method is stabilized, an "unstable name collision" lint is emitted. In the vast majority of circumstances, this is desirable.

However, when adding a new inherent method to the standard library, which deliberately shadows the name of a method in the Deref target, then this lint can be triggered, affecting users on stable who don't even know about the new unstable inherent method. As the new method is being added to the standard library, by the library team, it can be known that the lint isn't necessary, as the library team can ensure that the new inherent method has the same behaviour and won't cause any problems for users when it is stabilized.

pub struct Foo;
pub struct Bar;

impl std::ops::Deref for Foo {
    type Target = Bar;
    fn deref(&self) -> &Self::Target { &Bar }
}

impl Foo {
    #[unstable(feature = "new_feature", issue = "none", collision_safe)]
    pub fn example(&self) -> u32 { 4 }
}

impl Bar {
    #[stable(feature = "old_feature", since = "1.0.0")]
    pub fn example(&self) -> u32 { 3 }
}

// ..in another crate..
fn main() {
    let foo = Foo;
    assert_eq!(foo.example(), 3);
    // still invokes `Bar`'s `example`, as the `new_feature` isn't enabled, but doesn't
    // trigger a name collision lint (in practice, both `example` functions should
    // have identical behaviour)
}

Without this addition, the new inherent method would need to be insta-stable in order to avoid breaking stable users.

There wasn't a explicit solution described by the members of the library team (like in #99212) in any of the places that this was discussed, so I've done what seemed sensible to me. collision_safe needs to be used carefully, but I believe it solves the problem described without having any undesirable consequences. If there are alternative solutions then I'm happy to implement those instead.

cc @yaahc @rust-lang/libs-api

rust-highfive · 2022-07-29T09:09:23Z

r? @petrochenkov

(rust-highfive has picked a reviewer for you, use r? to override)

compiler/rustc_typeck/src/check/method/probe.rs

petrochenkov · 2022-07-29T14:04:30Z

@bors r+

bors · 2022-07-29T14:04:32Z

📌 Commit ca5a3a4a723cfb49156cec2282e3512fdb0267bf has been approved by petrochenkov

It is now in the queue for this repository.

davidtwco · 2022-07-29T14:06:06Z

Thanks @petrochenkov, I'd like to wait until someone from t-libs has a chance to sign-off on this being a desirable approach for them before we approve it :)

@bors r-

compiler/rustc_typeck/src/check/method/probe.rs

davidtwco · 2022-08-16T14:58:19Z

Most recent update from t-libs appears to be (cc @m-ou-se):

rust.tf/99898 typeck: silence lint for “collision safe” items
Could cause subtle issues when someone forgets to enable #![feature(..)] and they silently get the stable version, which might be more strict w.r.t. soundness.
Mara to comment.
source

I think that there are always going to be potential risks with silencing this lint, so it might not be desirable. I've tried to allievate the potential downsides as much as possible by limiting this to only when the library team has annotated the new unstable item (doing so when knowing which items it would shadow in std once stabilized) and when the item that is selected without the feature flag is from std (so user code that would be shadowed will always get a lint just like without this). That's still risky but hopefully it could be used sparingly and carefully, as in #97483. If there's another solution that the library team would like me to explore then I'm happy to do so.

m-ou-se · 2022-08-23T14:57:03Z

Apologies, I should've replied sooner.

As mentioned in the meeting notes you quoted, I'm a bit worried about users silently getting a different version of a method if they enable/disable a #![feature].

It seems to me that if it really doesn't matter which one they get, it'd be fine to insta-stabilize the new one, which removes the need for this PR. If it does matter which one of the two equally named methods is called, for example because one more quickly results in UB than the other (like in #97483) then we probably want to be careful and not make it easy to get the wrong one by forgetting to enable a feature.

davidtwco · 2022-08-23T15:23:05Z

If it does matter which one of the two equally named methods is called, for example because one more quickly results in UB than the other (like in #97483) then we probably want to be careful and not make it easy to get the wrong one by forgetting to enable a feature.

This makes sense to me. I guess I'm not entirely sure what the next steps are here. #97483 is an example where it does matter which method is called (as per the quoted comment), but also where you want to be able to introduce the new methods unstably (as per #97483 (comment)).

This pull request makes introducing the new methods unstably possible - but also introduces the possibility of enabling a feature flag and more quickly having UB or some other such difference in behaviour because a different method will be called. Given the nature of the lint being silenced, while there may be other ways this could be implemented which change how t-libs-api indicate the lint should be silenced, I can't see a way to enable unstably merging functions like these that doesn't also come with the associated risk that this approach does (though it's worth noting that the risk for any given pair of functions should be entirely predictable for t-libs-api).

Regardless, feel free to close this if you don't want the change or r=petrochenkov if you do, it's only here to enable merging PRs like #97483 if you want it :)

rustbot · 2022-09-13T08:52:41Z

rustc_error_messages was changed

cc @davidtwco, @compiler-errors, @JohnTitor, @estebank, @TaKO8Ki

When an unstable method exists on a type and the user is invoking a method that would no longer have priority when the unstable method is stabilized, an "unstable name collision" lint is emitted. In the vast majority of circumstances, this is desirable. However, when adding a new inherent method to the standard library, which deliberately shadows the name of a method in the `Deref` target, then this lint can be triggered, affecting users on stable who don't even know about the new unstable inherent method. As the new method is being added to the standard library, by the library team, it can be known that the lint isn't necessary, as the library team can ensure that the new inherent method has the same behaviour and won't cause any problems for users when it is stabilized. ```rust pub struct Foo; pub struct Bar; impl std::ops::Deref for Foo { type Target = Bar; fn deref(&self) -> &Self::Target { &Bar } } impl Foo { #[unstable(feature = "new_feature", issue = "none", collision_safe)] pub fn example(&self) -> u32 { 4 } } impl Bar { #[stable(feature = "old_feature", since = "1.0.0")] pub fn example(&self) -> u32 { 3 } } // ..in another crate.. fn main() { let foo = Foo; assert_eq!(foo.example(), 3); // still invokes `Bar`'s `example`, as the `new_feature` isn't enabled, but doesn't // trigger a name collision lint (in practice, both `example` functions should // have identical behaviour) } ``` Without this addition, the new inherent method would need to be insta-stable in order to avoid breaking stable users. Signed-off-by: David Wood <david.wood@huawei.com>

`collision_safe` works when the new-unstable item is only shadowing a from-std stable item, but it could also shadow a user-defined item from a extension trait, in which case the lint should still fire. Use the presence of a stability attribute on a chosen item as a heuristic for a std item and continue to lint if the chosen item isn't from std even if the unstable items are collision-safe. Signed-off-by: David Wood <david.wood@huawei.com>

bors · 2023-02-22T18:28:22Z

☔ The latest upstream changes (presumably #103042) made this pull request unmergeable. Please resolve the merge conflicts.

petrochenkov · 2023-02-22T20:53:58Z

Not sure what is the status here.

This change complicates method resolution logic and there's no obvious enthusiasm towards landing this, from what I see in this thread, so maybe it should be closed.
@rustbot author

davidtwco · 2023-02-23T09:21:29Z

I'm happy to close this, doesn't seem like there was interest in it.

rust-highfive assigned petrochenkov Jul 29, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jul 29, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 29, 2022

davidtwco mentioned this pull request Jul 29, 2022

Add as_ptr and as_mut_ptr inherent method to String #97483

Closed

petrochenkov reviewed Jul 29, 2022

View reviewed changes

compiler/rustc_typeck/src/check/method/probe.rs Outdated Show resolved Hide resolved

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 29, 2022

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 29, 2022

davidtwco force-pushed the stability-collision-safe branch from ca5a3a4 to fee95ea Compare July 29, 2022 14:09

davidtwco commented Jul 29, 2022

View reviewed changes

compiler/rustc_typeck/src/check/method/probe.rs Outdated Show resolved Hide resolved

yaahc added the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Aug 1, 2022

m-ou-se removed the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Aug 23, 2022